Reload Without User Extensions command #5620

lkcampbell · 2013-10-22T14:51:05Z

This PR addresses issue #5078.

lkcampbell · 2013-10-22T14:55:16Z

Note: the main.js diff makes the changes look more extensive than they really are. For example, I moved the position of _disableCache() to avoid JSLint error. All of the code is the same, though.

dangoor · 2013-10-22T17:23:33Z

I haven't looked at the code for this, but I just thought I'd add the drive-by comment that we should see how menus behave here because we had to quit when removing extensions in order to make sure their menu items went away. Unless something has changed, this could have the same issue.

lkcampbell · 2013-10-22T17:57:05Z

@dangoor, you are correct, it was an issue. That's why I had to implement removeMenu() and a few other things prior to submitting this PR.

This code now removes all menus prior to reloading the new Brackets instance and lets the reloaded code repopulate the menu system without the user extensions, effectively filtering out all user extension menu system changes.

marcelgerber · 2013-10-22T18:12:03Z

This works fine in general (also menu entries, I got many extensions installed which create menu entries), but after doing this there are no menu dividers left.
And as I'm our localization watchdog: We should provide localization for the menu entry :P

lkcampbell · 2013-10-22T19:29:10Z

@SAplayer, good catch, I didn't even notice that. Will have to investigate where the dividers are going.

lkcampbell · 2013-10-22T23:35:45Z

@SAplayer, menu dividers should show up correctly now. Can you test it again and verify it is working?

marcelgerber · 2013-10-23T14:05:08Z

Yes, confirmed fixed.

njx · 2013-10-28T22:19:35Z

@lkcampbell - fyi, I'm planning to get to this next sprint. Thanks.

njx · 2013-11-01T00:35:16Z

src/command/Menus.js

@@ -497,7 +498,8 @@ define(function (require, exports, module) {
                return;
            }
        } else {
-            brackets.app.removeMenuItem(menuItemID, function (err) {
+            dividerID = menuItemID.substring(menuItemID.indexOf("brackets"));


I don't fully understand the menu id code, but it looks like the issue here is that the menuItemID passed into removeMenuDivider includes the parent menu id at the beginning (because it's coming from the menuItemMap), so you need to strip that off. If so, then I'm a little concerned that just looking for indexOf("brackets") isn't safe, because it's possible that the parent menu id might contain the string "brackets". Perhaps it would be safer to use lastIndexOf("brackets") since we know that divider ids only have "brackets" at the beginning.

I think it would be better, though, to just redefine removeMenuDivider() to take the child menu id, and strip off the parent menu id in the loop that calls removeMenuDivider(), since you have that information there. It looks like removeMenuDivider() is actually a private function anyway (it's not in exports) so it should be fine to change the semantics of the parameter. (If you do, you should probably change the parameter name as well, for clarity.)

@njx, removeMenuDivider() is public, it is exposed through exports.Menu, but it's a pretty new API, I just wrote it to support removeMenu(). Regardless, I think it would be better to keep the parameter as is because the data object behind the menu items, menuItemMap, is keyed off of the full menuItemID. I would just have to rebuild the key name again from the child menu id for that code.

I think what is making this code feel odd is the fact that the native menu function brackets.app.removeMenuItem() takes the child menu id as its ID. All the other code pieces, like the HTML menus and the menuItemMap, take menuItemID as their ids. So, I have to do this weird id string hack to make it work with brackets.app.removeMenuItem().

I like your first idea. I will change to lastindexOf() and I will also look for a much more specific string "brackets-menuDivider-" which should be sufficient to prevent menu name collisions.

Makes sense.

njx · 2013-11-01T00:42:42Z

Done with initial review. I also noticed a couple of issues:

After doing Reload Without User Extensions, the dividers disappear from some of the menus (but not all).
Could we put Reload Without User Extensions immediately after Reload Brackets? (Right now "New Brackets Window" is between them.)

lkcampbell · 2013-11-01T16:09:09Z

@njx, I can't repro the issue you are seeing with the dividers disappearing. Can you tell me the exact menu location where it is happening? Also, if you have any extensions that happen to insert menu items or dividers near that problem area? Any console.log errors in Dev Tools? I only tested this in Windows so we should check Mac versus Windows as well I guess.

njx · 2013-11-01T18:24:52Z

Ah, yes, this is on Mac. It turns out that all of the dividers in Brackets-controlled menus disappear and don't reappear. (On the Mac, the application and Window menus are managed directly by the native shell, so the dividers in those are fine, but all the other Brackets-created menus lose their dividers.) This is true even if I don't have any extensions installed.

lkcampbell · 2013-11-01T18:42:11Z

Well, the good news is I am picking up a Mac this weekend so I can look at this problem soon.

@njx, this probably has something to do with that hacky divider fix we were discussing above. If you get some time today, can you see if the commit prior to the hacky fix works in Mac?

It would be at commit: https://github.com/lkcampbell/brackets/commit/9e6d0596ab5a10c41f589bafb1aa3a25d0b830f7

If you don't have time, that's fine, too, I will take a look this weekend.

…t move due to CodeMirror's indentation

…ode.

lkcampbell · 2013-11-02T05:26:00Z

@njx, I pushed the URLParams fixes and added a couple of useful API methods, remove() and isEmpty(). I used those fixes to fix the Param code per your suggestions.

I will see if I can get a unit test file for UrlParams together this weekend as well to test the old and new API methods.

I'm going to look at the divider problem with the Mac this weekend. Also, I will change the menu location of the Reload Without User Extensions command per your other suggestions.

I will wait until there is a fix for addIndicator() before adding the code to put an indicator in the status bar.

That should cover everything we discussed. Just wanted to get this entry in here tonight to keep track of the coding that still needs to be done on this.

ALF Automation

When autoindenting, only insert a tab if the cursor doesn't move due to CodeMirror's indentation

lkcampbell · 2013-11-03T16:01:13Z

@njx, I got my Mac this weekend and I can definitely reproduce the problem with the menu dividers disappearing with the Mac version.

I think I am going to need help from someone who understands the menu functionality in the Mac shell to fix this problem.

The original problem I had with dividers (and regular menu commands) disappearing was occurring when I used the wrong menu item id when calling brackets.app.removeMenuItem(). My best guess is the menu item still exists even though the menu is removed, so the next time the menu is repopulated, there is a conflict when the new menu item is added and it fails to be added. I don't know this for sure, I haven't seen any error logs. These are my observations from playing around with menu item ids on the JavaScript side of things.

At the very least, this problem repros in Mac but not in Windows, so the shell function behavior has different cross-platform behavior which should probably be addressed. So, is it possible I can get some assistance on this problem?

lkcampbell · 2013-11-03T16:44:35Z

@njx, I added the menu reorder you requested. I'm not sure why some of @redmunds changes got pulled in. Maybe something to do with how I pulled my branch into my new computer. Such is the eternal mystery of git. Hopefully that didn't mess anything up.

njx · 2013-11-06T01:37:13Z

Not sure who's most familiar with the Mac menu code at this point. Unfortunately my next couple of weeks are filling up so I might not be able to get this across the finish line in this sprint. I'll see if I can hand it off to someone else on the team--if not we might have to get it in next sprint.

lkcampbell · 2013-11-06T14:22:11Z

@njx, it's okay. This is just one of those features that has a knack for finding other issues :).

This PR is actually getting a bit cluttered anyway. I think what I will do is separate out the three issues we have discovered (Mac menu dividers, UrlParams bug and new methods, addIndicator() fix), address those separately, then close this PR and submit a new, clean PR when everything else has been addressed.

To close with a tautology, we will get it in when we get it in.

njx · 2013-11-06T17:54:20Z

Thanks @lkcampbell - very helpful to break up the work that way.

redmunds · 2013-12-19T18:43:00Z

@lkcampbell We committed to getting this pull request in to Sprint 35, but I now see it's been closed. Is this because you're waiting on pull request #6246? Anyway, I'm signed up for both of these, so let me know what's the next step. Thanks.

lkcampbell · 2013-12-19T23:59:01Z

@redmunds, correct, @njx and I were originally shooting for Sprint 35, but he found three problems (one of which is issue #6246) that I still need to address. I summarized the problems here:

#5078 (comment)

The biggest problem is figuring out why the menu dividers continue to disappear on the Mac. I still have to create a repro on this problem and file a bug.

This pull request will eventually go away. I am just keeping it around as a reference. The timeline on is indefinite until the other issues are addressed.

redmunds · 2013-12-20T01:18:24Z

@lkcampbell Let me know which branch to checkout and how to reproduce the problem, and I'll dig into the native code on Mac.

Add Reload Without User Extensions command

7436cb1

Bug fixes and code clean up

9e6d059

ghost assigned njx Oct 22, 2013

Fix menu divider bug and small code clean up

7f17970

Another small code clean up

d4f3b05

njx reviewed Nov 1, 2013
View reviewed changes

Narciso Jaramillo and others added 2 commits November 1, 2013 13:38

For #5068, when autoindenting, only insert a tab if the cursor doesn'…

4e66521

…t move due to CodeMirror's indentation

Fix bugs and add remove() and isEmpty() to UrlParams. Fix parameter c…

3a49c3d

…ode.

redmunds and others added 3 commits November 2, 2013 08:53

Merge pull request #5825 from adobe/alf_localization

a22c85d

ALF Automation

Merge pull request #5815 from adobe/nj/issue-5068

530a1e0

When autoindenting, only insert a tab if the cursor doesn't move due to CodeMirror's indentation

Merge remote-tracking branch 'origin/5078-fix' into 5078-fix

5de4559

Change menu item order

5531475

lkcampbell closed this Nov 6, 2013

ghost assigned redmunds Dec 19, 2013

lkcampbell mentioned this pull request Dec 29, 2013

Reload Without User Extensions #6334

Merged

lkcampbell deleted the 5078-fix branch March 25, 2014 02:20

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Reload Without User Extensions command brackets-archive/bracketsIssues#5153

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload Without User Extensions command #5620

Reload Without User Extensions command #5620

lkcampbell commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

dangoor commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

marcelgerber commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

marcelgerber commented Oct 23, 2013

njx commented Oct 28, 2013

njx Nov 1, 2013

lkcampbell Nov 1, 2013

njx Nov 1, 2013

njx commented Nov 1, 2013

lkcampbell commented Nov 1, 2013

njx commented Nov 1, 2013

lkcampbell commented Nov 1, 2013

lkcampbell commented Nov 2, 2013

lkcampbell commented Nov 3, 2013

lkcampbell commented Nov 3, 2013

njx commented Nov 6, 2013

lkcampbell commented Nov 6, 2013

njx commented Nov 6, 2013

redmunds commented Dec 19, 2013

lkcampbell commented Dec 19, 2013

redmunds commented Dec 20, 2013

Reload Without User Extensions command #5620

Reload Without User Extensions command #5620

Conversation

lkcampbell commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

dangoor commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

marcelgerber commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

lkcampbell commented Oct 22, 2013

marcelgerber commented Oct 23, 2013

njx commented Oct 28, 2013

njx Nov 1, 2013

Choose a reason for hiding this comment

lkcampbell Nov 1, 2013

Choose a reason for hiding this comment

njx Nov 1, 2013

Choose a reason for hiding this comment

njx commented Nov 1, 2013

lkcampbell commented Nov 1, 2013

njx commented Nov 1, 2013

lkcampbell commented Nov 1, 2013

lkcampbell commented Nov 2, 2013

lkcampbell commented Nov 3, 2013

lkcampbell commented Nov 3, 2013

njx commented Nov 6, 2013

lkcampbell commented Nov 6, 2013

njx commented Nov 6, 2013

redmunds commented Dec 19, 2013

lkcampbell commented Dec 19, 2013

redmunds commented Dec 20, 2013